Conversation
|
saturn-dbeal
left a comment
There was a problem hiding this comment.
Review: feat(builder): contract labels
Built and ran the test suite — 23 tests failing across 7 suites. The core feature (contract labels) looks good architecturally, and the schema cleanup is a nice improvement. But there are a few categories of issues to address before merge.
1. Tests need updating for labels: undefined in outputs (13 failures)
Every step that now stores config.labels in its output will emit labels: undefined when no labels are provided. Tests use toStrictEqual which catches this. Either:
- Omit
labelsfrom output when undefined (e.g....(config.labels && { labels: config.labels })) - Or update all test expectations to include
labels: undefined
The first approach is cleaner — it avoids polluting serialized state with undefined fields.
Affected: deploy.test.ts, invoke.test.ts, pull.test.ts, clone.test.ts, builder.test.ts
2. .strict().merge(sharedActionSchema) breaks strict validation (4 failures)
The pattern:
.strict()
.merge(sharedActionSchema);In Zod, .merge() after .strict() creates a new schema that no longer enforces strict mode. This means schemas silently accept unknown keys — the "fails when setting invalid value" tests all fail because { invalid: ["something"] } no longer throws.
Fix: apply .strict() after the merge, or compose differently:
.merge(sharedActionSchema)
.strict();Affected: deploy.test.ts, invoke.test.ts, pull.test.ts, clone.test.ts
3. artifactNameRegex is now stricter — breaks test fixtures (3 failures)
The new regex ^[A-Z]{1}[\w]+$ requires artifacts to start with uppercase. The old .refine() was more lenient. Test fixtures use lowercase artifact names like wohoo and greeter which now fail validation.
This is actually a reasonable tightening of validation since Solidity contracts do conventionally start with uppercase, but:
- The test data in
definition.test.tsusesartifact: "wohoo"— needs to be"Wohoo"or similar - The
builder.test.tsfixtures need the same treatment
4. Error message strings changed (2 failures)
Tests check for specific error substrings like "Package name exceeds 32 bytes" and "Package version exceeds 32 bytes" — the new regex-based validation produces different messages. Update test expectations to match the new messages.
5. varSchema lost fields
The old varSchema explicitly had defaultValue, description, and depends. The new version:
export const varSchema = z.object({}).catchall(z.string());uses catchall which accepts any string values, so description still works as a string. But depends (an array) would be rejected. Since sharedActionSchema is not merged here, var actions lose depends support. If var actions should support depends, merge the shared schema.
6. LSP src/index.ts looks like boilerplate
The new packages/lsp/src/index.ts appears to be the VS Code LSP example server nearly verbatim — it validates uppercase words and has placeholder completions (TypeScript/JavaScript). Presumably this is scaffolding for future work? If so, consider noting that in the PR description or a TODO comment. If it's meant to be functional, it needs the Cannon-specific logic.
7. Minor: include field added without schema validation
chainDefinitionSchema now has:
include: z.array(z.string()).describe("...").optional(),This is fine, but there's no corresponding runtime handling visible in this diff. If it's for a future PR, maybe note that.
Summary: The labels feature and schema cleanup are solid improvements. The main work needed is fixing the test suite — mostly mechanical updates. The .strict().merge() ordering is the most important semantic bug to fix.
No description provided.